fix: reject non-http public base URLs - #230
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe change centralizes public URL normalization, restricts URLs to HTTP(S), applies normalization during configuration loading and writing, and adds tests for invalid URLs, persisted configuration, wildcard hosts, and write safety. ChangesPublic URL normalization
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change consistently rejects unsupported public URL schemes and preserves the existing configuration when validation fails; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Greptile SummaryThis PR centralizes
Confidence Score: 5/5The PR appears safe to merge, with invalid public URL schemes consistently rejected before runtime use or persistence. The shared normalizer preserves existing HTTP(S) normalization behavior, rejects only unsupported schemes, and validates before filesystem mutation so failed writes retain the previous configuration.
|
| Filename | Overview |
|---|---|
| src/config.ts | Replaces the local URL parser with the shared HTTP(S)-enforcing normalizer while preserving effective-config precedence and host derivation. |
| src/user-config.ts | Adds shared public URL validation and normalizes non-null values before any persisted configuration write. |
| src/config.test.ts | Covers rejected FTP values from environment and persisted config, plus preservation of the prior file after a rejected write. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
E["Environment publicBaseUrl"] --> N["Normalize and require HTTP(S)"]
P["Persisted publicBaseUrl"] --> N
W["Configuration write"] --> N
N -->|Valid| C["Effective or persisted configuration"]
N -->|Invalid| R["Reject without overwriting config"]
Reviews (1): Last reviewed commit: "fix: reject non-http public base URLs" | Re-trigger Greptile
Summary
publicBaseUrlfrom persisted config orDEVSPACE_PUBLIC_BASE_URLdevspace config set publicBaseUrl ...cannot save an unsupported schemeWhy
Interactive setup already rejects non-HTTP(S) public URLs, but the persisted-config and environment-variable load paths accepted any scheme supported by
URL. For example,ftp://example.com/devspacecould be saved and later surfaced bydevspace doctoras an FTP MCP URL.This makes the HTTP(S) requirement a shared invariant across setup/config writes and runtime config loading.
Tests
Regression coverage verifies that:
DEVSPACE_PUBLIC_BASE_URLis rejectedconfig.jsonis rejectedPrior work
#37 previously included setter-side HTTP(S) validation as part of a much larger config-management change, but that PR was closed without merging and did not enforce the invariant in the runtime load path. This PR isolates that validation and applies it consistently to all current config sources.
Summary by CodeRabbit